Skip to content

Conversation

@arunas-cesonis
Copy link

@arunas-cesonis arunas-cesonis commented Apr 2, 2025

Bumps versions of Electron (from 20.3.3 to 35.1.2) and capnp (from 0.10.3 to 1.1.0) to allow using this library in newer Electron versions.

Tested by replacing previous version in a project that makes heavy use of this on Windows, Apple arm64 and x64.

The specific project that this change was tested on has shown a noticeable overall performance improvement on all platforms when moving from Electron 20.3.3 to 35.1.2 which may mean that any performance degradation (if any) due to extra allocation and memcpy introduced in this change should be negligible.

@arunas-cesonis arunas-cesonis requested review from a team and mwilliamson-fourier and removed request for a team April 3, 2025 07:40
@arunas-cesonis arunas-cesonis marked this pull request as ready for review April 3, 2025 07:41
Copy link

@mwilliamson-fourier mwilliamson-fourier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the Windows binary is checked directly into this repository, but (unless I missed it) it hasn't been updated. What platforms has this been tested on?

@arunas-cesonis
Copy link
Author

I believe the Windows binary is checked directly into this repository, but (unless I missed it) it hasn't been updated. What platforms has this been tested on?

Have only tested on Linux thus far

@arunas-cesonis
Copy link
Author

@mwilliamson-fourier Windows binary updated in f6833a1

Copy link

@mwilliamson-fourier mwilliamson-fourier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me! It might just be worth being explicit in the pull request / commit description about the testing that's been done, for instance that we've tested across the four platforms that we run this module on using an updated version of Electron, and overall performance is improved.

@arunas-cesonis
Copy link
Author

This seems good to me! It might just be worth being explicit in the pull request / commit description about the testing that's been done, for instance that we've tested across the four platforms that we run this module on using an updated version of Electron, and overall performance is improved.

I don't think my testing has shown that the performance of this module has improved!

@mwilliamson-fourier
Copy link

This seems good to me! It might just be worth being explicit in the pull request / commit description about the testing that's been done, for instance that we've tested across the four platforms that we run this module on using an updated version of Electron, and overall performance is improved.

I don't think my testing has shown that the performance of this module has improved!

I thought overall performance (of the application) had improved using the new version of Electron, including the new version of this module?

@arunas-cesonis
Copy link
Author

This seems good to me! It might just be worth being explicit in the pull request / commit description about the testing that's been done, for instance that we've tested across the four platforms that we run this module on using an updated version of Electron, and overall performance is improved.

I don't think my testing has shown that the performance of this module has improved!

I thought overall performance (of the application) had improved using the new version of Electron, including the new version of this module?

Sorry, didn't understand your comment fully. I still think we do not have knowledge of what are the changes in performance in this module, but updated the description to include my findings!

@arunas-cesonis arunas-cesonis merged commit b5428c8 into main Apr 10, 2025
1 check passed
@arunas-cesonis arunas-cesonis deleted the TF-905 branch April 10, 2025 12:10
@arunas-cesonis arunas-cesonis restored the TF-905 branch April 10, 2025 12:11
@arunas-cesonis arunas-cesonis deleted the TF-905 branch April 10, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants